Skip to content

Memoize version check #2274

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 12, 2017
Merged

Memoize version check #2274

merged 4 commits into from
Nov 12, 2017

Conversation

effigies
Copy link
Member

Fixes #2273.

Changes proposed in this pull request

  • Create PackageInfo template that reads version information from a file or command output, passes to a subclass-defined parser, and stores the result for future calls
  • Example subclasses for AFNI, ANTs and FreeSurfer

@satra
Copy link
Member

satra commented Nov 10, 2017

@effigies - should we use memoize here, or just call the version check again? in the unlikely, but possible scenario that someone changes a software on the system.

@effigies
Copy link
Member Author

The specific purpose is to memoize so that when calling an interface many times that runs a version check under the hood, we're not spawning an additional process each time.

Under what circumstances would you want to actively support changing versions during a run, and is it worth accounting for that situation by default? Most of the effects are going to be for input spec versioning, which will happen at workflow construction time. If you swap in an incompatible version after that, it should break when it's time to run. But significantly changing the environment is one of those cases where I feel like breaking is both expected and probably a good alternative to silently pressing on.

The one case I can see is if you're working in a Python console, and upgrade software, and for whatever reason you really don't want to start a new console. With this change, you could do interfaces.ants.base.Info._version = None, but we could also add a method or function to clear the cache.

@codecov-io
Copy link

codecov-io commented Nov 10, 2017

Codecov Report

Merging #2274 into master will increase coverage by 0.01%.
The diff coverage is 68.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2274      +/-   ##
==========================================
+ Coverage    72.5%   72.52%   +0.01%     
==========================================
  Files        1188     1188              
  Lines       59134    59135       +1     
  Branches     8505     8506       +1     
==========================================
+ Hits        42877    42889      +12     
+ Misses      14871    14860      -11     
  Partials     1386     1386
Flag Coverage Δ
#smoketests 72.52% <68.29%> (+0.01%) ⬆️
#unittests 70.14% <63.41%> (+0.01%) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/freesurfer/base.py 73.17% <100%> (+0.09%) ⬆️
nipype/interfaces/afni/base.py 54.83% <100%> (+0.93%) ⬆️
nipype/interfaces/ants/base.py 61.7% <36.36%> (+9.97%) ⬆️
nipype/interfaces/base.py 85.02% <73.91%> (-0.26%) ⬇️
nipype/pipeline/plugins/multiproc.py 78.16% <0%> (+1.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48c2b6b...adf5a35. Read the comment docs.

@satra
Copy link
Member

satra commented Nov 12, 2017

@effigies - i'll merge this in a second. the situation really deals with environment where people have long running jobs, but underlying software can get updated without people knowing. while this is not common, as i mentioned earlier, i think with versioning, just like file hashing we cannot rely on the external environment to remain constant.

@satra satra merged commit 9ec7458 into nipy:master Nov 12, 2017
oesteban added a commit to oesteban/nipype that referenced this pull request Nov 21, 2017
@effigies effigies deleted the memoize_version_check branch November 21, 2017 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants